Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an advisory for a potential DOS in toml-reader-0.1.0.0 #56

Merged

Conversation

david-christiansen
Copy link
Contributor

@david-christiansen david-christiansen commented Jun 19, 2023

This is my attempt to validate the process by creating an advisory as if I were not associated with the project. I've opened a couple of PRs and issues based on what I found. The rest of this PR test will be me role-playing through the process.


This is an advisory for an issue from toml-reader that I found some time ago. I reported it to the author of the package and he promptly fixed it. I emailed him about creating this advisory, and he's aware that I'm doing so and supports it.

See the advisory text for details, but the basic problem is that the parser for numbers takes time linear in the size of the denoted number. Because of exponent notation, this means that a string of length n can take O(10^n) time to parse.

I don't really understand CVSS, so I left it blank, hoping that the committee will help me create it. From what I can see, this is only a vulnerability if the library is used to parse untrusted input. If used for the common case, which is a configuration file, then the problem is no big deal, as someone being able to DOS their own machine doesn't let them hurt others. But the very tools that underlie this database use the library to parse TOML that's coming from others, so a DOS is indeed a concern.


Advisory

  • It's not duplicated
  • All fields are filled
  • It is validated by hsec-tools

hsec-tools

  • Previous advisories are still valid

@david-christiansen
Copy link
Contributor Author

I'm not sure what I'm doing wrong - I got the following validation error:

advisories/hackage/biscuit-haskell/HSEC-2022-0001.md: no error
Advisory structure error:
InvalidFormat "reference.type" "BUG REPORT"
advisories/hackage/toml-reader/HSEC-0000-0000.md: advisories/hackage/aeson/HSEC-2023-0001.md: no error
EXAMPLE_README.md: no error
EXAMPLE_ADVISORY.md: no error

The message seems to be telling me about lots of files that I didn't just create, plus the one that I did. I guess it means that the others are fine, but the one I created has a bad reference type?

@david-christiansen
Copy link
Contributor Author

Someone in an issue (#53) pointed out that the example advisory in the README has the allowed categories. I'll revise!

@david-christiansen
Copy link
Contributor Author

Meta comment: it might be worth making a little command-line app that gives a somewhat friendlier way to create these advisories. I think that most Haskell users don't really know what a CWE or CVSS is, and we'll most commonly get fairly minimal TOML blocks rather than people going out and reading up. One way to address this could be a little script in the repo that asks a few questions and guides them through this process.

@david-christiansen
Copy link
Contributor Author

My CI has now passed, but it looks to me like the job that validates the advisory didn't actually run! In particular, I don't see "Invoke check-advisories workflow" as one of the jobs behind the green check on 6cfeccc, but it's what failed on 6299888.

@frasertweedale
Copy link
Collaborator

My CI has now passed, but it looks to me like the job that validates the advisory didn't actually run! In particular, I don't see "Invoke check-advisories workflow" as one of the jobs behind the green check on 6cfeccc, but it's what failed on 6299888.

It did run on 6cfeccc: https://github.com/haskell/security-advisories/actions/runs/5315187694/jobs/9623351121

If I'm not mistaken, the hsec-tools check job does not appear until after the Haskell-CI matrix jobs have completed (successfully).

@david-christiansen
Copy link
Contributor Author

Ah, now I see it. I suppose I was just insufficiently patient (I hit reload a few times right after they stopped running and it had turned green and didn't see it). This was also in the evening, and coffee had not been consumed for some time. In any case, thanks!

@david-christiansen
Copy link
Contributor Author

Is there anything else I need to do with this report?

Copy link
Collaborator

@blackheaven blackheaven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually you need to rename it HSEC-2023-0004.md

BTW, it's something the bot should do.

@TristanCacqueray
Copy link
Collaborator

Renaming is annoying, the CONTRIBUTING.md doesn't explain how to work exactly. Perhaps we could adapt https://github.com/rustsec/advisory-db/blob/main/.github/workflows/assign-ids.yml ?

@frasertweedale
Copy link
Collaborator

frasertweedale commented Jul 13, 2023

Yeah, I think we could do something similar to RestSec. One thing I dislike in the RustSec approach is that advisories get merged to main with ID RUSTSEC-0000-0000, then the job kicks in and creates a second PR to assign the ID. I would prefer an approach that checks and updates the initial pull request, so that everything that lands on main has a valid ID. (This is important for the OSV export job, so that we do not generate and export bad data.)

I'll try and tackle this in the next week or so.

@david-christiansen
Copy link
Contributor Author

david-christiansen commented Jul 13, 2023

It looks to me like merge queues could solve this - it looks like a kind of integrated marge-bot or homu.

Perhaps a simple solution would be an action that assigns an ID (perhaps built by composing https://github.com/marketplace/actions/file-changes-action with a tiny blob of shell to rename new files named HSEC-0000-0000.md to the right thing), triggered by a PR being added to a merge queue?

Edit: Shell is not enough - we'll probably need some Haskell to reassign the ID in the TOML block. I suppose the command-line tools could get an argument that makes them assign missing IDs in the repo.

@david-christiansen
Copy link
Contributor Author

I renamed the file. I did it in two steps to see if the CI catches the mismatch between the filename and the internal ID :)

@david-christiansen
Copy link
Contributor Author

Back to my user role-play: How should I interpret this?

advisories/hackage/biscuit-haskell/HSEC-2023-0002.md: no error
advisories/hackage/xmonad-contrib/HSEC-2023-0003.md: no error
advisories/hackage/aeson/HSEC-2023-0001.md: no error
Advisory structure error:
IllegalOutOfBandOverride "date"
advisories/hackage/toml-reader/HSEC-2023-0004.md: EXAMPLE_README.md: no error
EXAMPLE_ADVISORY.md: no error

@frasertweedale
Copy link
Collaborator

frasertweedale commented Jul 13, 2023

Back to my user role-play: How should I interpret this?

advisories/hackage/biscuit-haskell/HSEC-2023-0002.md: no error
advisories/hackage/xmonad-contrib/HSEC-2023-0003.md: no error
advisories/hackage/aeson/HSEC-2023-0001.md: no error
Advisory structure error:
IllegalOutOfBandOverride "date"
advisories/hackage/toml-reader/HSEC-2023-0004.md: EXAMPLE_README.md: no error
EXAMPLE_ADVISORY.md: no error

You have to remove the date field from the TOML. The date of the advisory will be inferred from the Git history.

@frasertweedale
Copy link
Collaborator

Edit: Shell is not enough - we'll probably need some Haskell to reassign the ID in the TOML block. I suppose the command-line tools could get an argument that makes them assign missing IDs in the repo.

The rustsec-admin tool has a command to assign IDs to unassigned advisories. We can do something similar. It involves both editing the TOML and renaming files. Most likely I will just search-and-replace to edit the TOML rather than using the first-class TOML serialisation, to avoid field re-ordering (or the complexity we could incur to avoid it).

@frasertweedale
Copy link
Collaborator

I'll take over this PR. It will be the first advisory to include multiple affected libraries (base, toml-reader, ...?). We have some hsec-tools work to do to support that scenario. I propose to use symbolic links to reference it from multiple directories (with the hard copy in base, probably).

See also the discussion at https://gitlab.haskell.org/ghc/ghc/-/issues/23538

@frasertweedale
Copy link
Collaborator

I updated the PR - it now mentions both base and toml-reader as affected. It has also been assigned id HSEC-2023-0007

@frasertweedale
Copy link
Collaborator

Failed uniqueness check is expected, and will require rebase after #98 lands.

@frasertweedale frasertweedale merged commit 06fbe0b into haskell:main Jul 22, 2023
7 checks passed
@frasertweedale frasertweedale mentioned this pull request Jul 22, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants